@afshin If you are concerned about the amount of time searching the keyboard shortcuts during isToggled, it would be reasonably straightforward to cache the results of that calculation, and invalidate the cache if the keyboard shortcuts change.

I thought that with #4070 we were able to keep the same structure that currently exists because the line break behavior is correct. Doesn't that obviate the need for checking which shortcut is set?

No, this is mostly orthogonal: it allows people to choose whether they want to use Enter or Shift+Enter to execute code.

The fix from #4070 for line breaks works for both keybindings here.

Right, sorry, I hadn't re-read the code. This seems like a nice feature. I'm a little uncomfortable with the fact that you're writing a setting to another extension's key.

Do you think maybe it would be better to update the shortcuts extension to have a public method for saving/overwriting a binding? This seems like behavior other extensions may want even after we create a nice shortcut editor UI, so maybe we should offer it programmatically.

What do you think?

Yes, I agree that it is not ideal to write to the key of a different extension.

We could do what you suggest. Another option would be for the shortcuts-extension to listen to commands.keyBindingChanged(), and handle changes in the keybindings there. Then, when other extensions mess with keybindings they will automatically be serialized to the settings system.

@ian-r-rose: That's a reasonable solution as long as we make sure the shortcuts extension handles the case where it's the one adding the binding and ignores those!

I think we can put a change guard somewhere to make that work.

Yeah, absolutely. I was just pointing it out as a thing to look out for if that's how you want to implement it. I think listening to the keyBindingChanged changed in the shortcuts extension is a more robust solution than what I suggested. Do you want to do that in this PR?

Sure, I'll do that here.

Awesome.
👷

Hmm, this is actually a bit trickier than I had hoped. I am not sure how to remove an existing keybinding in an extension without a handle on the DisposableDelegate that belongs to the shortcuts extension. Any ideas around that?

I haven't tested this!

But something like this logic should do it, I think.

diff --git a/packages/shortcuts-extension/src/index.ts b/packages/shortcuts-extension/src/index.ts
index f4819054d..0f933f533 100644
--- a/packages/shortcuts-extension/src/index.ts
+++ b/packages/shortcuts-extension/src/index.ts
@@ -14,7 +14,7 @@ import {
 } from '@phosphor/commands';
 
 import {
-  JSONObject, JSONValue
+  JSONExt, JSONObject, JSONValue
 } from '@phosphor/coreutils';
 
 import {
@@ -57,7 +57,25 @@ const plugin: JupyterLabPlugin<void> = {
     const { commands } = app;
 
     settingReqistry.load(plugin.id).then(settings => {
+      // Load the shortcuts on initial load.
       Private.loadShortcuts(commands, settings.composite);
+
+      // Save newly modified key bindings to settings.
+      commands.keyBindingChanged.connect((sender: any, change: CommandRegistry.IKeyBindingChangedArgs) => {
+        const { args, command, keys, selector } = change.binding;
+        const binding = {
+          command, selector,
+          args: args as JSONObject,
+          keys: keys as Array<string>
+        };
+        const key = Object.keys(settings.composite).filter(key => {
+          return JSONExt.deepEqual(binding, settings.composite[key]);
+        })[0] || command;
+
+        settingReqistry.set(plugin.id, key, binding);
+      });
+
+      // Reload the shortcuts if settings change.
       settings.changed.connect(() => {
         Private.loadShortcuts(commands, settings.composite);
       });

Oh wait, I once again misunderstood your question. You're saying you can't remove an extant binding. Right.

It might be the case that offering a save method in the shortcuts extension is the way to go after all.

And possibly also listening to the keyBindingChanged as above in case somebody adds a binding without using that save method.

Right, only the owner of the keybinding seems to be able to remove it. I suppose we should go with some sort of save functionality.

Taking another look at this: I think we need a better story for best practices when it comes to keyboard shortcuts. Currently the shortcuts-extension owns the serialization of the shortcuts for all the core plugins. This is nice as they are all in one place, but it has a few holes in it:

• What if a plugin wants to do some additional, or more coordinated changes to shortcuts, as in this case? Should it write to the setting registry and let the shortcuts-extension plugin update commands.keyBinding? Or should it update commands.keyBinding and let the shortcuts-extension do the serialization?
• There is currently no good way for an extension to remove a keybinding. commands.addKeyBinding returns an opaque disposable, which is stored in a private namespace in the shortcuts-extension. The only way to do it at the moment is to write to the setting registry of shortcuts-extension and let it pick up the changes.
• We could think about exposing a Token in the shortcuts-extension with something like save and remove on it. However, this is kind of messy, and it feels like it would be the third place where similar functionality is implemented (commands.keyBindings, the setting registry, and the shortcuts-extension).
• If extensions should add new keybindings to the commands object, should we serialize those by default? Or should they be considered in-memory bindings?

Anyways, food for thought. I am not sure what the best way forwards is.

According to @ian-r-rose this needs a rebase and then review/merge.

@afshin This is rebased and ready for a look.